fix zap logger example and add tests #577
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
theruziev
force-pushed
the
fix_log_zap_example
branch
from
c53a440 to
1aeebc7
Compare
theruziev
force-pushed
the
fix_log_zap_example
branch
from
be5788a to
45b54fe
Compare
theruziev
force-pushed
the
fix_log_zap_example
branch
from
45b54fe to
796983b
Compare
|
Thanks for this and sorry for lag. Example was fixed, but I would love those tests! Do you mind rebasing? |
|
yep |
theruziev
force-pushed
the
fix_log_zap_example
branch
from
796983b to
70d7244
Compare
|
rebased, and returned examples function, maybe it's helpful to see how used it. |
There was a problem hiding this comment.
Thanks for this!
Tests are great (just I would move out of suite, but not a blocker).
However we kept InterceptorLogger adapters in test exactly so no one depends / import those. It's only for inspiration and copy. There is even comment suggesting that // This code is simple enough to be copied and not imported.. We have to enforce this otherwise people will start to depend on this module which is not meant to be specially versioned and compatibile (!).
Can we move those back?
| // Add any other interceptor you want. | ||
| ), | ||
| ) | ||
| // Output: | ||
| } | ||
|
|
||
| type kitExampleTestSuite struct { |
There was a problem hiding this comment.
Let's avoid "suite" patterns, especially for single method test. Why not single unit test?
There was a problem hiding this comment.
I used the test suite because that already has a server and client I can easily use for a test. I try to find another way to do that
There was a problem hiding this comment.
if you have any idea how to that this without test suite I will try to implement that.
There was a problem hiding this comment.
Let's avoid "suite" patterns, especially for single method test. Why not single unit test?
Hello, pardon my curiosity but I wonder why should we avoid suite pattern since it gave us a server and client inside?
It gave us expandable way to add more tests in the future.
There was a problem hiding this comment.
Test suite types in Go like this are uncommon and unnecessary. If you need a server in a test, you can just create one. If you want to isolate the logic, create a function. Test suites take OOP beyond what is usually necessary in Go. Just write test functions, skip the suite.
theruziev
force-pushed
the
fix_log_zap_example
branch
from
2814228 to
02bbafe
Compare
|
I returned interceptors to test files |
theruziev
force-pushed
the
fix_log_zap_example
branch
from
02bbafe to
6f07a83
Compare
|
i work on it |
Changes
Before:
After:
Verification
I added test for testing/verification